Introduces a new LogicalType: FILE#585
Conversation
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
Looks reasonable to me a few minor comments for clarification.
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
Just a few questions I have after reviewing the Rust implementation.
| The referenced bytes are compressed with the same `CompressionCodec` as the one | ||
| specified for the `inline` column. |
There was a problem hiding this comment.
Let's make this clearer.
| The referenced bytes are compressed with the same `CompressionCodec` as the one | |
| specified for the `inline` column. | |
| The bytes referenced by a self-reference are compressed with the same `CompressionCodec` as the one specified for the `inline` column. |
There was a problem hiding this comment.
Should we point out CompressionCodec can differ per page?
There was a problem hiding this comment.
@alkis thoughts? Is there a reason that the data needs to be compressed additionally? Most formats may be optimal anyway.
There was a problem hiding this comment.
The reason is because we are making the assumption that the type of data stored in the file column is homogeneous. I argue that's a good assumption. If this column contains text, there is little reason to assume it will be a string when small and an image when large. Ergo if the writer chose to compress the column it should compress the self-referenced packed blobs too.
I wanted to make all packed references compressed as well but there was some pushback for that with the argument that external may be referenced by different parquet files and it may choose its own compression scheme (or none at all). I can see arguments both ways - it is a tradeoff.
Making all packed references inherit the CompressionCodec makes the spec more consistent.
Making only self-references inherit the CompressionCodec is friendlier to shared external references.
We pick one of the two, it is a tradeoff.
There was a problem hiding this comment.
Should we point out CompressionCodec can differ per page?
|
@etseidl @sfc-gh-sgrafberger @alkis @rdblue addressed your comments, can you please take another look? |
sfc-gh-sgrafberger
left a comment
sfc-gh-sgrafberger
left a comment
There was a problem hiding this comment.
Thank you @brkyvz! Looks good to me.
…a-io#585) Align with the latest apache/parquet-format#585: - size must be set whenever offset is set; a self-reference (no path) must set offset, and therefore size. Drop the now-invalid [offset, EOF) and [0, size) self-reference modes and the external [offset, EOF) mode from the resolution table; add explicit invalid rows. - Define "set" (present, non-null, non-empty for strings) and allow sparse group definitions (a group need only define the fields it uses); add an inline-only example group. - Fields matched case-sensitively by name; field IDs "if they exist". - Readers should ignore unknown checksum algorithms. Follows the consistent prose intent of PR delta-io#585; note its resolution table still lists an [offset, EOF) row that contradicts its own validation section (offset requires size) -- to be raised on the Parquet PR. Co-authored-by: Isaac
| #### Fields | ||
|
|
||
| For the descriptions below, a field is *set* when it is present in the `FILE` group | ||
| and its value is non-null (and, for string fields, non-empty). A field is *not set* |
There was a problem hiding this comment.
Nit: we should be clear that implementations are not expected to treat empty strings as null.
14 participants
Rationale for this change
Introduces a new type called File as a typed FileReference. The design document is here.
The motivation is as follows:
What changes are included in this PR?
Introduces the specification for FileType.
Do these changes have PoC implementations?
Yes: